Skip to content

[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 정콩이 미션 제출합니다.#1126

Open
xxbeann wants to merge 44 commits intowoowacourse:xxbeannfrom
xxbeann:step2
Open

[🚀 사이클2 - 미션 (블랙잭 게임 실행)] 정콩이 미션 제출합니다.#1126
xxbeann wants to merge 44 commits intowoowacourse:xxbeannfrom
xxbeann:step2

Conversation

@xxbeann
Copy link

@xxbeann xxbeann commented Mar 14, 2026

INTRO

안녕하세요 영이! 이번 리뷰도 잘 부탁드립니다 🙇🏻‍♀️
부족한만큼 많이 배워가겠습니다!

체크 리스트

  • 미션의 필수 요구사항을 모두 구현했나요?
  • Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
  • 애플리케이션이 정상적으로 실행되나요?

개요

이번 PR에서는 블랙잭 게임에 배팅 금액, 블랙잭 판정, 최종 수익 계산/출력 기능을 추가했습니다.
기능 구현 과정에서 도메인 책임이 흩어지지 않도록 패키지 구조와 객체 책임도 함께 리팩터링했습니다.
특히 이번 사이클에서는 불변 객체 개념을 적용해보려 했습니다.
배팅 금액과 수익은 상태를 변경하는 대신 새로운 값을 생성하는 방향으로 모델링했고,
이를 BettingAmount, Revenue에 반영했습니다.

변경 배경

기존에는 게임 진행, 결과 계산, DTO 변환, 입력 검증 등의 책임이 여러 객체에 분산되어 있었습니다.
이번 변경에서는 다음 방향을 기준으로 구조를 정리했습니다.

  • 참가자/카드/게임/배팅 도메인 책임 분리
  • 입력/출력 계층이 도메인 규칙을 직접 알지 않도록 정리
  • 게임 결과와 수익 계산을 분리해 확장 가능하도록 개선
  • 최종 요구사항인 "참가자들의 최종 수익 출력"까지 흐름 연결

주요 변경 사항

1. 배팅 금액 입력 및 검증 흐름 추가

  • 플레이어별 배팅 금액 입력 기능 추가
  • 입력 검증 로직을 별도 validator로 분리
  • InputView가 직접 도메인 규칙을 알지 않도록 정리

2. 참가자/카드 관련 도메인 구조 정리

  • Players 일급 컬렉션 도입
  • Players 내부에서 플레이어 수, 중복 이름 검증 수행
  • 외부에서 내부 컬렉션을 꺼내지 않도록 forEach 위임 메서드 추가
  • Cards/ParticipantCards 책임 분리
  • Participant, GameManager의 DTO 의존 제거 (이는 아직 의존 방향이 완전히 해결되지 않았습니다.)

3. 패키지 분리

도메인 객체를 역할에 따라 아래와 같이 재구성했습니다.

  • domain.card
  • domain.participant
  • domain.game
  • domain.betting

이를 통해 카드, 참가자, 게임 결과, 배팅/수익 계산 책임을 분리했습니다.

4. 블랙잭 판정 및 결과 확장

  • Participant에 블랙잭 판정 로직 추가
  • GameResultBLACKJACK 결과 추가
  • 플레이어와 딜러가 동시에 블랙잭일 경우 무승부 처리하도록 수정
  • 결과 계산 책임을 GameManager에서 GameResultManager로 이동

5. 배팅/수익 도메인 추가

최종 수익 계산을 위해 관련 도메인을 도입했습니다.

  • BettingAmount: 배팅 금액 VO
  • BettingAmounts: 참가자별 배팅 금액을 관리하는 일급 컬렉션
  • Revenue: 최종 수익 VO
  • Revenues: 참가자별 최종 수익을 관리하는 일급 컬렉션
  • CalculateProfit: GameResultBettingAmount를 바탕으로 수익 계산
  • GameResultManager: 참가자 결과 및 수익 집계 담당

배팅 금액 자체와 정산/수익 계산 책임을 분리하려고 했습니다.

6. 최종 수익 출력 기능 추가

  • 컨트롤러에서 도메인 결과를 ParticipantRevenueDto로 변환
  • OutputView에서 참가자 및 딜러의 최종 수익 출력
  • 딜러 수익은 참가자 수익 합을 기준으로 계산(zeroSum)

7. 테스트 정리 및 보강

  • 패키지 구조 변경에 맞춰 테스트 위치 재구성
  • Players, ParticipantCards, Player, Dealer 등 도메인 테스트 정리
  • BettingAmountTest, CalculateProfitTest, GameResultManagerTest 추가
  • 기존 예외 메시지 비교 테스트를 예외 타입 검증 중심으로 정리
  • 테스트 메서드명을 한글 기반으로 통일

참고

기능 구현과 함께 책임 분리를 병행한 PR이라 변경 범위가 다소 큽니다.
리뷰 시에는 아래 순서로 봐주시면 편합니다.

  1. 패키지 분리 및 도메인 책임 변경
  2. 블랙잭/수익 계산 로직
  3. 컨트롤러-DTO-View 출력 흐름
  4. 테스트 재구성

어떤 부분에 집중하여 리뷰해야 할까요?

  • GameResultManager가 게임 결과 계산과 수익 집계를 함께 가지는 현재 책임 범위가 적절한지
  • BettingAmounts, Revenue 등 배팅/수익 도메인 분리가 자연스러운지
  • 컨트롤러에서 DTO 변환을 담당하는 현재 흐름이 적절한지
  • 딜러 수익을 참가자 수익의 합의 반대값으로 계산하는 방식이 요구사항에 부합하는지

xxbeann added 29 commits March 12, 2026 23:49
players일급 컬렉션을 도입해 도메인 객체 스스로 검증하게 구현했습니다.
input view 에서 도메인 규칙을 모르게끔 리팩토링하였고,
gameManager에 흩어져 있던 검증로직을 관련 도메인으로 끌어왔습니다.
- inputView 테스트를 위해 기존에 쓰던 inputStream을 제거하고, validator를 분리 이를 테스트 했습니다.
- 기존 exception의 메시지를 테스트 하던 방식을 예외 타입 검증으로 변경했습니다. 추후 커스텀 예외를 통해 메시지 타입까지 비교할 예정입니다.
- 기존에 있던 DisplayName과 영어 메서드명을 한글 메서드명으로 변경했습니다.
- Players 일급 컬렉션을 생성한 후 순회하는 로직을 구현하기 위해 일급 컬렉션 안에 forEach 순회 메서드를 만들었습니다.
이는, player 리스트의 forEach를 위임한 역할입니다.
- 처음에는 getPlayers를 통해 순회를 해결하려 했으나, 그럴 경우 캡슐화가 약해져 외부가 Players내부 표현 방식에 의존한다는 걸 알았습니다.
외부가 players 내부 구현을 알 경우 변경에 취약하고, 책임이 새어나가 확장에 불리하다 판단해 forEach를 도입했습니다.
- 실제로 리팩터링 과정 중 눈에 띄게 가독성이 향상됨을 체감했습니다.
- Cards를 일급 컬렉션으로 생성후, 이 Cards를 ParticipantCards와 Deck이 필드로 갖게 했습니다.
기존 저는 card는 단일 1장, cards는 card의 복수형, Deck은 cards의 특수한 상황(52장 + shuffle)으로 생각했습니다.
cards가 일급 컬렉션의 의도였으나 changeAvailableAceCount필드를 추가하게 되면서 player가 들고 있는 Cards로 일급 컬렉션이 전락했습니다.
영이의 말대로 Deck과 Cards가 각각 카드 리스트를 가지는 일급 컬렉션의 구조를 생각해 보았으나, 저는 순수하게 카드 리스트를 가지는 일급 컬렉션 cards를 고려했고.
이를 다시 Deck과 ParticipantCards에서 사용되게 했습니다. 기존에 있던 Cards를 ParticipantCards로 메서드 명을 바꾸면서 최대한 전체 구조에 수정을 줄이고자 했습니다.
실제로 순수한 일급 컬렉션 Cards를 도입해보니 ParticipantCards와 Cards의 책임이 어느정도 분리됨을 확인할 수 있었습니다.
다만, 영이의 설계와 제가 한 설계중 뭐가 더 나은 방향인지 궁금합니다!
제가 처음에 ParticipantCards에 getSize등의 메서드 들이 있었다가, 안써서 지웠는데, 굳이 꼽자면 나중에 확장성이 있겠다...정도...?
- BettingManager 도입으로 참가자별 배팅 금액 관리
- BettingAmount에 0원/승리 배팅 금액 계산 로직 추가
- Participant의 이름 반환 타입을 Name 객체로 변경
- 배팅 금액 관련 단위 테스트 추가
- Participant에 블랙잭 판정 로직 추가
- ParticipantCards에 카드 개수 조회 기능 추가
- BettingManager에 블랙잭 승리 정산 기능 추가
- BettingAmount에 1.5배 계산 로직 추가
- 블랙잭 및 배팅 정산 관련 테스트 보강
- BettingAmount와 BettingManager에서 정산 관련 메서드 제거
- GameResult에 BLACKJACK 결과 추가
- Player에 블랙잭 결과 판정 로직 반영
- 수익 계산용 CalculateProfit, Revenue 도메인 추가 준비
- CalculateProfit과 GameResultManager로 참가자 수익 계산 로직 추가
- BettingAmount와 Revenue를 값 객체로 보강
- RevenueManager 도입으로 수익 집계 기반 마련
- 테스트 패키지를 도메인 구조에 맞게 재배치
- CalculateProfit 테스트 추가 및 기존 배팅 테스트 정리
- BlackJackGameController에서 BettingManager 초기화 로직 추가
- GameManager의 결과 조회 책임을 GameResultManager로 이동
- GameResultManager에 플레이어/딜러 기반 결과 계산 로직 추가
- GameResultManager 테스트 추가 및 기존 GameManager 테스트 정리
- ParticipantRevenueDto 추가
- 컨트롤러에서 도메인 수익 결과를 DTO로 변환하도록 변경
- GameResultManager에서 참가자 및 딜러 수익 집계 로직 추가
- OutputView에 최종 수익 출력 기능 추가
- Dealer 이름을 고정값으로 관리하도록 정리
System.out.println();
System.out.println(player + "의 배팅 금액은?");
Scanner sc = new Scanner(System.in);
Integer input = sc.nextInt();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

askBettingAmount()에서 입력을 처음에는 String으로 받아 문자열 검증 후 int로 변환하는 방향을 생각했습니다.
다만 Scanner.nextInt()를 사용하면 숫자가 아닌 입력에 대해 기본적인 검증 효과를 바로 얻을 수 있어서,
구현 복잡도를 줄이기 위해 현재는 nextInt()를 사용하는 방식으로 변경했습니다.

이 경우 입력 계층에서 별도의 문자열 검증 로직을 두지 않아도 된다는 장점이 있다고 봤는데,
이런 상황에서 String -> 검증 -> 변환 흐름을 유지하는 편이 더 낫다고 보시는지,
아니면 nextInt()를 활용해 입력 단계의 복잡도를 줄이는 방향도 괜찮다고 보시는지 의견이 궁금합니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextInt를 사용하고 숫자가 아닌 문자를 입력했을때
어떤 에러메세지가 보이던가요??
사용자가 쉽게 이해하기 좋은 메세지가 내려오는지를 판단해보면 좋을것 같습니다

import org.junit.jupiter.api.Test;

class NameTest {
// TODO: 나중에 커스텀 예외로 변경. 현재는 예외가 터지는 것만 검증됨.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception 의 메세지를 테스트로 하는것은 좋은 방법은 아닐것 같아요, 에러메세지는 자주 바뀔수 있고 값이 추가되는등 변경사항이 잦은 부분일것 같습니다!

리뷰 주신 내용을 반영해 현재는 예외 메시지 비교 대신 예외 타입만 검증하도록 테스트를 리팩터링했습니다.

다만 이렇게 변경하고 보니, IllegalArgumentException만으로는 "예외가 발생했다"는 사실은 확인할 수 있지만,
정말 의도한 상황에서 적절한 예외가 발생했는지까지는 충분히 드러나지 않는다는 고민이 들었습니다.

이런 경우를 구분하기 위해 보통 커스텀 예외를 도입하게 되는지 궁금합니다.
즉, 메시지 자체를 테스트에 고정하지 않으면서도 예외의 의미를 더 명확하게 검증하려면
커스텀 예외로 책임을 분리하는 방식이 일반적인지 의견을 듣고 싶습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커스텀 예외는 정말 특수한 예외인경우에 대해서만 만드는 편인것 같아요.
프론트에서 별도로 Exception 타입을 보고 처리가 필요한 경우요

실제 대부부의 회사들은 그냥 input으로 잘못들어온건 프론트에서 별다른 처리 없이(여기서는 view) 사용자에게 메세지만 잘 전달되면 되기 때문에 하나의 에러로 간주해서 처리할것 같습니다!

Comment on lines +1 to +9
package view;

public class InputValidator {
public static void validateContinueResponse(String input) {
if (!input.matches("[yn]")) {
throw new IllegalArgumentException("응답은 y와 n만 허용됩니다.");
}
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

입력 검증 책임을 InputView에서 분리해 InputValidator를 만들고, 해당 검증 로직을 별도로 테스트하도록 변경했습니다.

리뷰에서 InputStream을 직접 바꿔가며 테스트하는 방식은 권장하지 않는다고 말씀해주셨는데,
그 이유가 System.setIn()이 JVM 전역 상태를 변경하기 때문인지 궁금합니다.

제가 이해한 바로는, 이런 방식은 테스트 수가 많아졌을 때 전체 테스트 실행 과정에서 다른 테스트와 상태를 공유하게 되고,
특히 병렬 실행 환경에서는 예기치 않은 실패나 race condition 같은 문제로 이어질 수 있을 것 같습니다.

즉, InputStream 기반 테스트를 지양하는 주된 이유가
전역 상태 변경으로 인해 테스트 격리가 깨지고, 실행 순서나 동시성에 영향을 받을 수 있기 때문인지 확인받고 싶습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흠 저도 깊게 고민해보지 못한 포인트네요
제 관점에서는 view는 사실 프론트의 영역이고 백엔드는 도메인 테스트에 집중하자가 포인트였습니다.
보통은 System.setIn() 과 같은 코드까지 짜면서 테스트를 하는 케이스를 본적이 없어서 익숙하지 않음에 리뷰 드렸습니다.

위와 같은 문제도 충분히 발생할수 있을것 같네요! 학습하신 내용으로도 충분히 이런 테스트는 하지 않는 방향이 좋을것 같습니다!

Comment on lines +17 to +19
public void forEach(Consumer<Player> action) {
players.forEach(action);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Players를 일급 컬렉션으로 도입한 뒤, 내부 컬렉션 표현을 외부에 노출하지 않기 위해 forEach 메서드를 추가했습니다.
(getPlayers를 쓰면 캡슐화가 약해진다. 바깥에서 내부구현 List에 의존하게 된다.)
이 과정에서 리팩터링 방향을 잡거나 익숙하지 않은 문법을 적용할 때 AI 도움을 일부 받았습니다.

예를 들어 forEach 메서드 사용해서 리팩토링 해줘! 이런식으로
구조를 바꾸는 과정에서 AI에게 구현 방향이나 스켈레톤 코드 예시를 요청하는 방식으로 사용했습니다.

이럴 때 AI를 활용하는 것이 학습에 실제로 도움이 되는 방식인지,
아니면 익숙하지 않은 문법이나 리팩터링은 직접 더 많이 손으로 써보는 쪽이 낫다고 보시는지 궁금합니다.

특히 이런 경우,
"AI에게 바로 구현을 맡기는 것"과
"AI에게 방향/힌트만 받고 직접 구현하는 것"
사이에서 어떤 방식으로 활용하는 것이 학습에 더 도움이 되는지도 의견을 듣고 싶습니다.
익숙하지 않은 문법은 어떤 식으로 학습을 해야할까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forEach를 이렇게 쓰는것보다 상황에 맞는 메서드를 직접 구현하는게 좋다고 생각하빈다.
consumer를 통해서 어떤값이 들어올지 모르고 외부에서 players에 대해서 아무값이나 변경이 가능할것 같아서요.

학습을 하는데 있어서는 AI에게 바로 구현을 하기보다는 먼저 구현을 해보고 AI와 토론을 해보는게 좋을것 같습니다.
예전에는 익숙하지 않은 언어나 기능들을 학습하기 위해서는 Test 코드를 통해 이것저것 연습해보고 했었는데
지금은 AI를 이용하여 어떻게 학습할수 있을지 고민해보면 좋을것 같습니다. 이런 질문들도 AI에게 한번 물어보세요!

Comment on lines +6 to +11
public class Cards {
private final List<Card> cards;

public Cards(List<Card> cards) {
this.cards = new ArrayList<>(cards);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cards 라는 클래스가 Deck에도 쓰이고 Participant에도 쓰이고 있는것 같아요
현실에서는 같은 카드들일지라도 객체의 관점에서는 deck의 cards와 participant의 cards는 다를것 같아요 deck은 카드를 draw하는 역할이고 participant의 카드들은 추가하고 점수를 계산하는 역할이 있을것 같은데요 이 경우에 어떻게 일급컬렉션을 활용할 수 있을지 고민해보면 좋을것 같아요 다음 사이클 진행을 위해 제 생각을 먼저 공유하자면 Deck 자체로 List 를 감싸고 있는 일급컬렉션이라고 생각합니다 Deck의 필드로는 List 가 있어야하고 participant가 가지고 있는게 Cards 가 되어야하지 않을까 싶습니다.

리뷰 주신 방향을 반영해 구조를 다시 고민해봤습니다.

처음에는 Card를 단일 카드 1장, Cards를 Card의 복수형, Deck을 52장의 카드와 셔플/드로우 규칙을 가진 Cards의 특수한 상황으로 생각했습니다. 그래서 Cards를 먼저 일급 컬렉션으로 두고, 이를 DeckParticipantCards가 내부적으로 사용하는 구조를 생각했습니다.

다만 구현 과정에서 에이스 점수 계산을 위해 changeAvailableAceCount 같은 상태가 들어가면서,
기존 Cards가 순수한 컬렉션이라기보다 사실상 참가자 카드 책임까지 함께 가지는 형태가 되었다고 느꼈습니다.
그래서 이후에는 Cards는 순수하게 카드 리스트를 감싸는 역할로 두고,
참가자의 카드 관리와 점수 계산 책임은 ParticipantCards로 분리하는 방향으로 정리해봤습니다.

이렇게 바꾸고 보니 CardsParticipantCards의 책임이 이전보다 어느 정도 분리된다고 느꼈습니다.
다만 여전히 고민되는 부분은,
리뷰에서 말씀해주신 것처럼 Deck 자체가 카드 리스트를 직접 가진 일급 컬렉션이 되고,
참가자는 별도의 Cards 또는 ParticipantCards를 가지는 구조가 더 자연스러운지입니다.

즉 현재 제가 적용한 방향과,
리뷰에서 제안해주신 "Deck과 Participant 쪽 컬렉션 책임을 더 명확히 분리하는 방향" 중에서
어느 쪽이 더 객체지향적으로 자연스럽고 확장에 유리한지 의견을 듣고 싶습니다.

추가로, 초기에 ParticipantCardsgetSize() 같은 메서드도 두었다가 사용하지 않아 제거했는데,
이런 메서드들은 현재 필요가 없더라도 이후 확장 가능성을 보고 남겨두는 편이 좋은지,
아니면 실제 사용 시점에 추가하는 편이 더 나은지도 궁금합니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParticipantCards, Deck 각각이 List 를 필드로 가진 일급컬렉션 각각 클래스가 되어야할것 같습니다.
Cards라는 클래스는 현재 구조에서 필요없을것 같아요
객체 클래스의 depth만 깊어져서 오히려 복잡성이 증가하고 있는 구조인것 같습니다

에이스 점수 계산을 위해서 changeAvailableAceAcount가 있는구조가 좋은 방향인지도 고민해보면 좋을것 같아요
점수 계산을 할때 ace 여부를 체크하면 되지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요!
Cards를 별도 일급 컬렉션으로 둔 이유는, DeckParticipantCards가 모두 카드 리스트를 다루고 있기 때문에
장기적으로 공통 컬렉션 추상화가 있으면 확장에 유리할 수 있다고 판단했기 때문입니다.

다만 리뷰를 보면서,
현재 구조에서는 이런 공통 추상화가 확장성을 높이기보다 오히려 depth만 늘리고
불필요한 추상화가 될 수 있다는 관점도 이해하게 되었습니다.

그래서 이 부분에 대해 한 가지 더 여쭤보고 싶습니다.

저는 "미래에 공통 로직이 늘어날 가능성"을 보고 Cards를 유지하려 했는데,
영이는 현재 시점에서는 DeckParticipantCards의 책임이 이미 다르기 때문에
굳이 같은 컬렉션 추상화로 묶지 않는 편이 더 자연스럽다고 보신 것 같았습니다.

혹시 이런 경우에 공통 추상화를 남길지, 각 도메인이 직접 리스트를 가지게 할지 어떤 기준으로 판단하시는지 궁금합니다.

예를 들어

  • 단순히 내부적으로 같은 List<Card>를 가진다는 사실만으로는 부족한지
  • 실제로 공유하는 책임/행위가 충분히 안정적일 때만 추상화를 남겨야 하는지
  • 혹은 현재 요구사항에서 직접 쓰이지 않는 확장성은 일단 접어두고, 필요해졌을 때 추출하는 편을 선호하시는지

같은 판단 기준이 궁금합니다.

추후에는 말씀해주신 방향으로도 직접 리팩터링해보면서 구조를 비교해볼 생각입니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cards 라는 클래스에서 추후 공통로직으로 쓰일만한게 어떤게 있나요??
제가 생각했을때 지금 Cards 의 메서드들은 공통적인 기능이라기보다 List 에서 기본적으로 제공되는 기능들을 한번더 호출하는 역할 밖에 없는것 같아요
추후 공통적인 기능이 생긴다는 영역도 List 에서 제공되는걸 한번더 메서드 depth만 늘릴것 같다고 생각합니다!

Comment on lines +18 to +20
return cards.stream()
.map(Card::getCardInfo)
.toList();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스트림을 사용하는 방향에 대해서도 의견을 듣고 싶습니다.

리팩터링 과정에서 일부 컬렉션 순회 로직을 스트림으로 바꿔볼 수 있겠다고 느낀 부분이 있었는데,
개인적으로는 아직 for문이나 forEach 기반 순회가 더 직관적으로 읽히는 경우가 많다고 느꼈습니다.

그래서 모든 순회 로직을 스트림으로 바꾸는 것이 꼭 더 좋은 방향인지,
아니면 가독성과 의도가 더 잘 드러나는 경우에는 일반 반복문을 유지하는 것도 괜찮은지 궁금합니다.

또 실제 현업에서는 컬렉션 처리 시 스트림을 기본적으로 많이 사용하는 편인지,
아니면 팀/상황에 따라 반복문과 스트림을 구분해서 사용하는지도 함께 여쭤보고 싶습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상황에 따라 더 가독성, 성능 등을 잘 고려해서 판단하시는게 좋을것 같습니다!
모든 상황에서 스트림이 좋다고 forEach, for문이 좋다고 할수는 없을것 같아요
딱 정의해서 말할수 없는것 같고 개발을 진행하면서 판단력을 길러보시면 좋을것 같습니다!

Comment on lines +27 to +36
void 참가자가_승리하면_수익은_배팅금액만큼_증가한다() {
Name name = new Name("pobi");
Player player = new Player(name);
Map<Name, BettingAmount> amountMap = new HashMap<>();
amountMap.put(player.getName(), new BettingAmount(1000));
BettingAmounts bettingAmounts = new BettingAmounts(amountMap);
CalculateProfit calculateProfit = new CalculateProfit(bettingAmounts);
int expected = 1000;
assertEquals(expected, calculateProfit.calculate(name, GameResult.WIN).getMoney());
}
Copy link
Author

@xxbeann xxbeann Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 범위 설정에 대해서도 의견을 듣고 싶습니다.

요구사항을 구현할 때는 가능한 한 작은 단위부터 TDD를 시작하려고 했습니다.
예를 들어 loseBettingAmount()처럼 메서드 단위에서
"플레이어가 배팅 금액을 잃는다", "플레이어가 수익을 돌려받는다" 같은 규칙을 먼저 검증하는 방식입니다.

그런데 구현을 진행하다 보니,
결국 상위 객체에서 여러 도메인이 함께 협력하는 흐름을 테스트하게 되면서
단위 테스트라기보다 얇은 통합 테스트에 가까운 형태로 올라가게 되었습니다.

이런 흐름이 자연스러운건지 궁금합니다.
(BettingAmount를 만들고, 수익을 계산해야하니 CalculateProfit을 만들고, 어 그런데 한 명이 아니라 여러명이네?
그럼 BettingAmounts도 있어야겠네 해서 만들고....그러다보니 정작 테스트하는 건 CalculateProfit에서 하고...)

이 과정에서
작은 단위 테스트를 먼저 쌓고, 이후 협력 객체를 묶는 얇은 통합 테스트까지 가는 흐름이 자연스러운지 궁금합니다...
(특히 GameResultManagerTest가 그런식으로 구현되어 있습니다.)

특히 상위 테스트가 통과하면 하위의 여러 작은 메서드들도 간접적으로 함께 검증되는 느낌이 드는데,
이럴 때 작은 단위 테스트와 협력 테스트의 경계를 어떻게 잡는 것이 좋은지도 의견을 듣고 싶습니다.

뭔가 메서드 단위로 시작해야지! 하고 시작하는데 돌아보면 결국 기능단위 인 것 같아서 고민이네요....

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작은 단위에서 테스트가 된것들은
상위 레벨에서는 과감하게 테스트 하지 않아도 된다고 생각합니다.

추가적으로요구사항이 메서드의 길이를 10줄 이하로 하라는것 때문에 개행없이 이렇게 처리하신걸까요??
가독성이 많이 떨어지는것 같습니다!
10줄이하는 가독성을 올리기 위해서인데 거기에 매몰되어 오히려 가독성을 해치는게 아닐지 고민해보면 좋을것 같습니다

또 테스트에서 중복이 많은 부분들을 어떻게 처리해 코드를 줄일수 있을지도 고민해보면 좋겠네요

Comment on lines +5 to +11
public class BettingAmount {
private final Integer money;

public BettingAmount(Integer bettingAmount) {
validateMinus(bettingAmount);
this.money = bettingAmount;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불변객체는 setter메서드를 제공하지 않으며, 내부상태를 제공하는 메서드는 제공하지 않거나, 방어적복사를 통해 제공한다. 또한, 생성자를 통해 초기화 된 후 GC에 의해 소멸되기 전까지 값이 변하지 않는다. 예시로는 String이 있다. 한편, final 변수를 통해 불변객체를 달성할 수 있다고 착각하지만 아니다. final은 재할당이 안되지만, 값은 변한다. final로는 달성할 수 없는 불변객체를 일급 컬렉션과 래퍼 클래스(VO)를 통해 달성하게 해준다.

제가 이번에 불변객체를 도입하면서 공부해본 내용입니다. 제가 이해한 방향이 적절한지 먼저 확인받고 싶습니다.

또 한편으로는 이번에 불변 객체를 공부하고 적용해보면서, 좋은 설계라는 이유로 불변성 자체에 과하게 집착하게 될 수도 있겠다는 느낌이 들었습니다. (이전에 저는 아키텍처 패턴에 강하게 집착한적이 있어서 그 때와 비슷한 느낌이 들었던 것 같네요)

그래서 실제 현업에서는 불변 객체를 어느 정도까지 적용하는 것을 목표로 하는지, 그리고 "여기서는 불변으로 두는 것이 적절하다"와 "여기서는 실용적으로 가변을 허용해도 된다"를 어떤 기준으로 판단하는지 의견을 듣고 싶습니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불변객체는 사용할 수 있으면 사용하는게 좋을것 같아요
실무에서는 인스턴스의 필드가 최소 10개는 되는데 매번 객체를 새롭게 생성해서 반환하는건 현실적으로 어렵기 때문에 불변을 많이 사용하지는 않긴합니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 단순 궁금증인데, 객체지향 생활체조에 따르면 인스턴스 필드를 3~4개로 유지하라고 하잖아요??
그러데 실무에서는 그럼에도 불구하고 10개정도로 유지가 되는 주된 이유가 무엇인가요??
현실과 코딩 사이에 항상 괴리가 발생하나요??

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현실적으로 해당원칙을 지키려면 많은 클래스를 가져야하는데
그 클래스를 관리하고 이해하는것보다 하나의 클래스에서 처리하는게 더 이해하기 쉽고 간단한것 같아요

예시를 찾아보니 Company를 위 원칙으로 지키기위해서는 6개의 클래스가 별도로 필요하더라고요
DB에 저장될때 entity는 결국 하나일텐데 매핑해주는 과정도 쉽지않을것 같고요 그리고 이후 사용하겠지만 jpa를 사용하다면 도메인 = entity로 사용하는 경우도 많아서 쉽지 않을것 같습니다

// 3개 이상의 인스턴스 변수를 필드로 가지는 Company
public class Company{
    private Long id;
    private String name;
    private String tel;
    private String ceoName;
    private String address;
    private String addressDetail;
    private String zipcode;
}
// 2개 이하의 인스턴스 변수로 구성된 객체 간의 관계로 표현한 Company
public class Company{
    private Long id;
    private BaseInfo baseInfo;
}

public class BaseInfo {
    private String name;
    private AdditionalInfo additionalInfo;
}

public class AdditionalInfo {
    private Contact contact;
    private Location location;
}

public class Contact {
    private String tel;
    private String ceoName;
}

public class Location {
    private Address address;
    private String zipcode;
}

public class Address {
    private String main;
    private String detail;	
}

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 정콩이
현재 핵심 요구사항이 TODO로 적혀있고 구현이 안되어 있는것 같습니다
몇가지 코멘트를 남기긴했는데 같이 고민해보시면서
핵심 요구사항을 구현하여 TODO를 없애고 다시 리뷰요청 주시면 좋을것 같습니다!

@@ -0,0 +1 @@
package domain.card;public class Card { private final Shape shape; private final Number number; public Card(Shape shape, Number number) { this.shape = shape; this.number = number; } public int getScore() { return number.getValue(); } // TODO: DTO 책임 분리 public String getCardInfo() { return number.getDisplayName() + shape.getShape(); } public boolean isAce() { return this.number.equals(Number.ACE); }} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 체크해봐주세요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LineFeed 오류가 있었던 것 같습니다!
반영했습니다!

import java.util.Objects;

public class Revenue {
private final int money;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

돈과 관련된 타입이 int가 적절할지 고민해보면 좋을것 같습니다

  1. 엄청 큰 돈이 들어올경우
  2. 달러 등 소수점이 있는경우
  3. 부동소수점 문제

위 세가지 문제에 대해 고민을 해보고 판단해보시면 좋을것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 주신 내용을 바탕으로 돈 타입과 계산 방식에 대해 다시 생각해봤습니다 🤔

  1. 큰 금액 처리
    현재는 int를 사용하고 있어 정수 overflow 가능성이 있습니다.
    이 경우 배팅 금액의 상한을 도메인 규칙으로 두는 방향이 나을지,
    아니면 타입 자체를 long으로 변경해 표현 범위를 넓히는 방향이 더 적절할지 고민하고 있습니다.

  2. 소수점이 있는 화폐
    현재는 원화 기준의 정수 금액만 가정하고 있지만,
    달러처럼 소수점 단위를 가지는 화폐까지 고려한다면 지금 구조는 한계가 있다고 느꼈습니다.
    그래서 과제 범위 안에서는 100원 단위처럼 입력 단위를 제한하는 도메인 규칙을 두는 방식도 가능하다고 생각했습니다.

  3. 부동소수점 문제
    double/float는 소수를 2진수로 완전히 정확하게 표현하지 못해 계산 오차가 생길 수 있다는 점도 함께 고민하게 되었습니다.
    현재 1.5는 2진수로 표현이 가능하기에 오류가 드러나지는 않지만,
    돈 계산을 double 기반 상수에 의존하는 방식은 확장성이나 안정성 측면에서 아쉽다고 느꼈습니다.

그래서 현재는 PROFIT_RATEBigDecimal로 두고 계산 로직을 리팩터링하는 방향도 고려하고 있습니다.

이렇게 변경방향을 고려하고 있는데, 더 나은 방식이 있을까요???

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

돈과 관련된것들은 실무에서도 대부분 아니 100% BigDecimal 을 사용하고 있을거에요
더 나아가면 참고로 money라는 개념은 currency와 함께 항상 같이 다녀야하는 의미로 대부분 사용하고 있을거에요

public class Money {
    private final BigDecimal amount;
    private final String currency;
}

위와 같은 형태로 많은곳에서 쓰이고 있을텐데 참고해보시면 좋을것 같습니다.(여기서 currency까지 적용은 오버엔지니어링이라 생각합니다)
https://shopify.dev/docs/api/admin-graphql/latest/objects/MoneyV2
실제 shopify와 같은 글로벌 기업들이 어떤 타입으로 Money를 관리하고 있는지 참고하면 좋을것 같네요

Comment on lines +18 to +20
return cards.stream()
.map(Card::getCardInfo)
.toList();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상황에 따라 더 가독성, 성능 등을 잘 고려해서 판단하시는게 좋을것 같습니다!
모든 상황에서 스트림이 좋다고 forEach, for문이 좋다고 할수는 없을것 같아요
딱 정의해서 말할수 없는것 같고 개발을 진행하면서 판단력을 길러보시면 좋을것 같습니다!

Comment on lines +27 to +33
public int sumScore() {
int sum = 0;
for (Card card : cards) {
sum += card.getScore();
}
return sum;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 부분은 stream 이 좋을것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영했습니다!!

Comment on lines +25 to +28
private Dealer initDealer() {
Dealer dealer = new Dealer();
return dealer;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불필요한 메서드 아닐까요 생성자에서 생성하면 될것 같네요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 그러네요!!!
감사합니다 👍

Comment on lines +35 to +37
private void distributeCardToDealer(Dealer dealer) {
distributeInitialCards(dealer);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 굳이 메서드 분리를 할필요가 있을지 고민해보면 좋을것 같습니다

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 그래도 Dealer에게 초기 카드를 분배한다는 의도를 더 담고 싶어서 distributeCardToPlayers와 동일하게 한 번 더 감쌌던 건데, 굳이 안 그래도 의미가 잘 전달이 되나요?? 지금은 아래와 같이 리팩토링한 상태입니다!

    public void distributeInitialCards() {
        distributeInitialCards(dealer);
        distributeCardToPlayers(players);
    }

    private void distributeCardToPlayers(Players players) {
        players.forEach(this::distributeInitialCards);
    }

    private void distributeInitialCards(Participant participant) {
        drawCardTo(participant);
        drawCardTo(participant);
    }

Comment on lines +5 to +11
public class BettingAmount {
private final Integer money;

public BettingAmount(Integer bettingAmount) {
validateMinus(bettingAmount);
this.money = bettingAmount;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불변객체는 사용할 수 있으면 사용하는게 좋을것 같아요
실무에서는 인스턴스의 필드가 최소 10개는 되는데 매번 객체를 새롭게 생성해서 반환하는건 현실적으로 어렵기 때문에 불변을 많이 사용하지는 않긴합니다.

}

private Revenue calculateDealerRevenue(Map<Name, Revenue> revenues) {
int dealerRevenue = -revenues.values().stream()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"-" 를 이렇게 붙이는 방식은 가독성도 떨어지고 의미전달을 놓치기 쉬운 코드인것 같아요
-1을 곱하거나 java.Math의 메서드등을 활용하여 좀더 가독성 좋은 방향을 고민해보면 좋을것 같습니다!

Copy link
Author

@xxbeann xxbeann Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하, 그렇군요!
처음에는 딜러의 수익이 참가자들 수익 총합의 반대 부호라 단순히 -를 붙이는 게 직관적일 것이라고 생각했었습니다.

하지만 리뷰어님 말씀대로 긴 스트림 체이닝 앞에 단항 연산자가 있으니 시각적으로 놓치기 쉽고,
다른 사람이 읽을 때 가독성이 떨어질 수 있다는 점도 공감이 가네요

조언해주신 방향을 바탕으로 플레이어들의 총 수익을 구하는 부분을 변수로 먼저 분리하고, Math.negateExact() 를 사용하여 '수익을 반전시킨다'는 의도가 코드에 더 명확히 드러나도록 수정했습니다. 좋은 피드백 감사합니다! 👍
(해당 메서드가 정수 오버플로우도 예외처리해주는군요? 배워갑니다 ㅎㅎ)

Comment on lines +6 to +11
public class Cards {
private final List<Card> cards;

public Cards(List<Card> cards) {
this.cards = new ArrayList<>(cards);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParticipantCards, Deck 각각이 List 를 필드로 가진 일급컬렉션 각각 클래스가 되어야할것 같습니다.
Cards라는 클래스는 현재 구조에서 필요없을것 같아요
객체 클래스의 depth만 깊어져서 오히려 복잡성이 증가하고 있는 구조인것 같습니다

에이스 점수 계산을 위해서 changeAvailableAceAcount가 있는구조가 좋은 방향인지도 고민해보면 좋을것 같아요
점수 계산을 할때 ace 여부를 체크하면 되지 않을까요?

Comment on lines +19 to +39
public GameResult judgeResult(Dealer dealer) {
if(this.isBlackJack() && dealer.isBlackJack()) {
return GameResult.DRAW; // TODO: 배팅 금액을 돌려 받는다
}
if(this.isBlackJack()) {
return GameResult.BLACKJACK; // TODO: 배팅 금액 1.5배
}
if (this.isBust()) { // TODO: 배팅 금액을 모두 잃는다.
return GameResult.LOSE;
}
if (dealer.isBust()) { // TODO: 배팅 금액을 받는다.
return GameResult.WIN;
}
if (this.getScore() > dealer.getScore()) {
return GameResult.WIN; // TODO: 배팅 금액을 받는다.
}
if (this.getScore() < dealer.getScore()) {
return GameResult.LOSE; // TODO: 배팅 금액을 모두 잃는다.
}
return GameResult.DRAW; // TODO: 배팅 금액을 돌려 받는다.
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 사이클2의 핵심 요구사항인것 같은데 현재 구현이 안되어있는걸까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구현되어 있었는데 제가 깜빡하고 주석을 제거 안했네요 ㅜㅜ
그냥 단순 궁금증인데 예를 들어 추후 구현 혹은 리팩터링 사항이다. 이런 내용도 프로덕션 코드에서는 주석을 안남겨 놓나요??
뭔가 제가 생각했을 땐 코드에 대한 설명이나 의도를 주석으로 남겨도 괜찮지 않을까 생각하는데, 오히려 주석없이 메서드명만으로 정확한 의도를 전달할 수 있어야 한다고 들어서요.
하지만 같은 메서드라도 사람마다 생각하는 게 다르니, 주석으로 저맥락 설명을 적어놓는 게 협업하는 게 낫지 않을까..라는 주관적인 생각입니다..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드를 보고 이해하기 어렵거나 쉽게 알기 어려운 부분에 대해서는 주석을 남겨놓습니다.
하지만 여기에서의 로직이 코드상으로 이해하기 어렵거나 한 부분은 없을것 같아요. 특이한 로직이 있는것도 아니고요!

주석은 코드가 변경되어도 같이 변경하는걸 까먹기 쉽기 때문에 웬만하면 작성하지 않는 방향으로 가는게 옳은 방향일것 같습니다.

@xxbeann
Copy link
Author

xxbeann commented Mar 15, 2026

영이 안녕하세요! ✨
빛보다 빠른 리뷰 덕분에 초안에서 고민하던 부분들을 일찍 해결할 수 있었네요! 🙌
피드백 주신 내용들 반영했고, 고치면서 든 궁금증들도 코멘트로 남겨보았습니다.
제가 쓴 내용이 조금 길어서 부담스러우실까 살짝 걱정되지만... 😅 편하실 때 가볍게 읽고 편하게 의견 보태주시면 정말 큰 도움 될 것 같습니다! 항상 많이 배우고 있습니다. 감사합니다!! 🙏

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정콩이 빠르게 반영해주셨네요
질문 주신내용과 추가적인 코멘트 남겼으니
확인해보시고 한번더 피드백 반영부탁드려요!

Comment on lines +5 to +11
public class BettingAmount {
private final Integer money;

public BettingAmount(Integer bettingAmount) {
validateMinus(bettingAmount);
this.money = bettingAmount;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현실적으로 해당원칙을 지키려면 많은 클래스를 가져야하는데
그 클래스를 관리하고 이해하는것보다 하나의 클래스에서 처리하는게 더 이해하기 쉽고 간단한것 같아요

예시를 찾아보니 Company를 위 원칙으로 지키기위해서는 6개의 클래스가 별도로 필요하더라고요
DB에 저장될때 entity는 결국 하나일텐데 매핑해주는 과정도 쉽지않을것 같고요 그리고 이후 사용하겠지만 jpa를 사용하다면 도메인 = entity로 사용하는 경우도 많아서 쉽지 않을것 같습니다

// 3개 이상의 인스턴스 변수를 필드로 가지는 Company
public class Company{
    private Long id;
    private String name;
    private String tel;
    private String ceoName;
    private String address;
    private String addressDetail;
    private String zipcode;
}
// 2개 이하의 인스턴스 변수로 구성된 객체 간의 관계로 표현한 Company
public class Company{
    private Long id;
    private BaseInfo baseInfo;
}

public class BaseInfo {
    private String name;
    private AdditionalInfo additionalInfo;
}

public class AdditionalInfo {
    private Contact contact;
    private Location location;
}

public class Contact {
    private String tel;
    private String ceoName;
}

public class Location {
    private Address address;
    private String zipcode;
}

public class Address {
    private String main;
    private String detail;	
}

import java.util.Objects;

public class Revenue {
private final int money;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

돈과 관련된것들은 실무에서도 대부분 아니 100% BigDecimal 을 사용하고 있을거에요
더 나아가면 참고로 money라는 개념은 currency와 함께 항상 같이 다녀야하는 의미로 대부분 사용하고 있을거에요

public class Money {
    private final BigDecimal amount;
    private final String currency;
}

위와 같은 형태로 많은곳에서 쓰이고 있을텐데 참고해보시면 좋을것 같습니다.(여기서 currency까지 적용은 오버엔지니어링이라 생각합니다)
https://shopify.dev/docs/api/admin-graphql/latest/objects/MoneyV2
실제 shopify와 같은 글로벌 기업들이 어떤 타입으로 Money를 관리하고 있는지 참고하면 좋을것 같네요

Comment on lines +6 to +11
public class Cards {
private final List<Card> cards;

public Cards(List<Card> cards) {
this.cards = new ArrayList<>(cards);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cards 라는 클래스에서 추후 공통로직으로 쓰일만한게 어떤게 있나요??
제가 생각했을때 지금 Cards 의 메서드들은 공통적인 기능이라기보다 List 에서 기본적으로 제공되는 기능들을 한번더 호출하는 역할 밖에 없는것 같아요
추후 공통적인 기능이 생긴다는 영역도 List 에서 제공되는걸 한번더 메서드 depth만 늘릴것 같다고 생각합니다!


private static Revenue calculateRevenue(GameResult gameResult, BettingAmount amount) {
if (gameResult == GameResult.BLACKJACK) {
return new Revenue((int) (amount.getMoney() * PROFIT_RATE));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 계산 결과를 int로 강제 타입 변경하는것은 안티 패턴중 하나일것 같아요
게임 로직적으로 방어가 되고있는지는 모르겠지만 bettingAmount가 3이고 1.5를 곱하게되면 4.5가 아닌 4가 나오겠네요!

return new Revenue(amount.getMoney());
}
if (gameResult == GameResult.LOSE) {
return new Revenue(-amount.getMoney());
Copy link

@choijy1705 choijy1705 Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 -를 앞에 붙이는 방법도 좋지 않을것 같아요

Comment on lines +1 to +9
package view;

public class InputValidator {
public static void validateContinueResponse(String input) {
if (!input.matches("[yn]")) {
throw new IllegalArgumentException("응답은 y와 n만 허용됩니다.");
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흠 저도 깊게 고민해보지 못한 포인트네요
제 관점에서는 view는 사실 프론트의 영역이고 백엔드는 도메인 테스트에 집중하자가 포인트였습니다.
보통은 System.setIn() 과 같은 코드까지 짜면서 테스트를 하는 케이스를 본적이 없어서 익숙하지 않음에 리뷰 드렸습니다.

위와 같은 문제도 충분히 발생할수 있을것 같네요! 학습하신 내용으로도 충분히 이런 테스트는 하지 않는 방향이 좋을것 같습니다!


public static Integer askBettingAmount(String player) {
System.out.println();
System.out.println(player + "의 배팅 금액은?");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scanner 와 같은 것들은 매번 생성하기보다
static하게 처리하는게 더 좋을것 같네요!

System.out.println();
System.out.println(player + "의 배팅 금액은?");
Scanner sc = new Scanner(System.in);
Integer input = sc.nextInt();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextInt를 사용하고 숫자가 아닌 문자를 입력했을때
어떤 에러메세지가 보이던가요??
사용자가 쉽게 이해하기 좋은 메세지가 내려오는지를 판단해보면 좋을것 같습니다

Comment on lines +27 to +36
void 참가자가_승리하면_수익은_배팅금액만큼_증가한다() {
Name name = new Name("pobi");
Player player = new Player(name);
Map<Name, BettingAmount> amountMap = new HashMap<>();
amountMap.put(player.getName(), new BettingAmount(1000));
BettingAmounts bettingAmounts = new BettingAmounts(amountMap);
CalculateProfit calculateProfit = new CalculateProfit(bettingAmounts);
int expected = 1000;
assertEquals(expected, calculateProfit.calculate(name, GameResult.WIN).getMoney());
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작은 단위에서 테스트가 된것들은
상위 레벨에서는 과감하게 테스트 하지 않아도 된다고 생각합니다.

추가적으로요구사항이 메서드의 길이를 10줄 이하로 하라는것 때문에 개행없이 이렇게 처리하신걸까요??
가독성이 많이 떨어지는것 같습니다!
10줄이하는 가독성을 올리기 위해서인데 거기에 매몰되어 오히려 가독성을 해치는게 아닐지 고민해보면 좋을것 같습니다

또 테스트에서 중복이 많은 부분들을 어떻게 처리해 코드를 줄일수 있을지도 고민해보면 좋겠네요

import org.junit.jupiter.api.Test;

class NameTest {
// TODO: 나중에 커스텀 예외로 변경. 현재는 예외가 터지는 것만 검증됨.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커스텀 예외는 정말 특수한 예외인경우에 대해서만 만드는 편인것 같아요.
프론트에서 별도로 Exception 타입을 보고 처리가 필요한 경우요

실제 대부부의 회사들은 그냥 input으로 잘못들어온건 프론트에서 별다른 처리 없이(여기서는 view) 사용자에게 메세지만 잘 전달되면 되기 때문에 하나의 에러로 간주해서 처리할것 같습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants